Skip to content

docs: clarify that H.edges[idx] returns attributes, not members#727

Merged
leotrs merged 2 commits into
mainfrom
docs-edges-getitem
May 28, 2026
Merged

docs: clarify that H.edges[idx] returns attributes, not members#727
leotrs merged 2 commits into
mainfrom
docs-edges-getitem

Conversation

@leotrs

@leotrs leotrs commented May 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Summary

A recurring point of confusion is that H.edges[0] returns the (empty) attribute dict rather than the members of edge 0. The behavior is technically correct, but the docstring didn't direct users to the actual member-access methods.

This PR keeps the behavior the same (per our v1.0 stance of hardening rather than changing the API) and improves the docstring:

  • Upfront note saying explicitly that this returns attributes, not members
  • Concrete examples showing both the empty default and a case with attributes set
  • See Also pointing at EdgeView.members and NodeView.memberships

Test plan

  • Doctests pass (run with --doctest-modules)
  • Full test suite passes (415 passed, 6 skipped)

@leotrs @claude
docs: clarify that H.edges[idx] returns attributes, not members
76e3e92 
A common confusion is that H.edges[0] returns the empty attribute dict
rather than the members of edge 0. The behavior is correct but the
docstring did not point users at the actual member-access methods. Add
an upfront note and concrete examples showing both attribute access and
the alternatives (.members, .memberships).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@leotrs leotrs force-pushed the docs-edges-getitem branch from 404b639 to 76e3e92 Compare May 28, 2026 06:42
@codecov

codecov Bot commented May 28, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.68%. Comparing base (b7a899d) to head (2c92ffb).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #727   +/-   ##
=======================================
  Coverage   93.68%   93.68%           
=======================================
  Files          66       66           
  Lines        5207     5213    +6     
=======================================
+ Hits         4878     4884    +6     
  Misses        329      329

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@maximelucas

maximelucas commented May 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Great idea to clarify this, thanks Leo.
I wonder though in what context the user will actually see the docs of the .getitem() method?
Like do we get there by typing H.nodes and then in notebook shift+tab to see the docs, or ..?
This is just to make sure that users will see this explanation easily and often.

@leotrs @claude
docs: also flag attr-vs-member confusion in NodeView/EdgeView class docs
2c92ffb 
H.nodes? and H.edges? are common discovery moves; surface the
attr-vs-member distinction in the class docstrings where users will
actually encounter it (per Max's review on #727).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@leotrs

leotrs commented May 28, 2026

Copy link
Copy Markdown
Collaborator Author

Good point Max. Strictly speaking the __getitem__ docstring only shows up via H.edges.__getitem__? or on the API reference page, not via the more natural H.edges[0]? (which evaluates first and shows the dict's docstring instead).

Pushed a follow-up that also adds a short attr-vs-member note to the NodeView and EdgeView class-level docstrings. Now H.nodes? / H.edges? (a common discovery move) surfaces the distinction too.

@maximelucas

maximelucas commented May 28, 2026

Copy link
Copy Markdown
Collaborator

Great!

@leotrs leotrs merged commit bd8a39e into main May 28, 2026
17 checks passed
@maximelucas

maximelucas commented May 29, 2026

Copy link
Copy Markdown
Collaborator

Oops, this was merged into main rather than dev. @leotrs @kaiser-dan ?
Btw, by default a new PR merges into main, can we change a setting so that the default is into dev?

@leotrs

leotrs commented May 29, 2026

Copy link
Copy Markdown
Collaborator Author

Sorry about that, totally my mistake — I missed the dev/main convention. Just retargeted #728 and #729 to dev, backmerged main into dev to recover the stranded commits, and opened #734 to document the workflow in CLAUDE.md so it doesn't happen again. Going to change the default branch to dev now so PRs target dev by default.

This was referenced May 29, 2026
Merged
Merged
@maximelucas maximelucas deleted the docs-edges-getitem branch June 1, 2026 06:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maximelucas maximelucas maximelucas approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@leotrs @maximelucas